From: Matthieu Gallien Date: Wed, 23 Apr 2025 09:54:34 +0000 (+0200) Subject: fix(rename): handle complex rename/move scenario X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2^2~8^2~1 X-Git-Url: https://dgit.raspbian.org/%22http:/www.example.com/cgi/%22https://%22Program/%22http:/www.example.com/cgi/%22https:/%22Program?a=commitdiff_plain;h=47b3a5c8ca5a4c50dcf48b73275555db790342e3;p=nextcloud-desktop.git fix(rename): handle complex rename/move scenario ensure we do not leak records and properly update them in client database Signed-off-by: Matthieu Gallien --- diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 002480c82..59b7ec9c9 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -302,8 +302,9 @@ void PropagateLocalRename::start() const auto previousNameInDb = propagator()->adjustRenamedPath(_item->_file); const auto existingFile = propagator()->fullLocalPath(previousNameInDb); const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget); + const auto originalFile = propagator()->fullLocalPath(_item->_originalFile); - const auto fileAlreadyMoved = !FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) && FileSystem::fileExists(existingFile); + const auto fileAlreadyMoved = (!FileSystem::fileExists(originalFile) || !FileSystem::fileExists(existingFile))&& FileSystem::fileExists(targetFile); auto pinState = OCC::PinState::Unspecified; if (!fileAlreadyMoved) { auto pinStateResult = vfs->pinState(propagator()->adjustRenamedPath(_item->_file)); @@ -315,6 +316,8 @@ void PropagateLocalRename::start() // if the file is a file underneath a moved dir, the _item->file is equal // to _item->renameTarget and the file is not moved as a result. qCDebug(lcPropagateLocalRename) << _item->_file << _item->_renameTarget << _item->_originalFile << previousNameInDb << (fileAlreadyMoved ? "original file has already moved" : "original file is still there"); + qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(originalFile) ? "original file exists" : "orignal file is missing") << originalFile << _item->_originalFile; + qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(existingFile) ? "existing file exists" : "existing file is missing") << existingFile << previousNameInDb; Q_ASSERT(FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) || FileSystem::fileExists(existingFile)); if (_item->_file != _item->_renameTarget) { propagator()->reportProgress(*_item, 0); @@ -444,7 +447,7 @@ void PropagateLocalRename::start() if (fileAlreadyMoved && !deleteOldDbRecord(previousNameInDb)) { return; - } else if (!deleteOldDbRecord(_item->_originalFile)) { + } else if (!deleteOldDbRecord(previousNameInDb)) { qCWarning(lcPropagateLocalRename) << "Could not delete file from local DB" << _item->_originalFile; return; } @@ -470,25 +473,28 @@ void PropagateLocalRename::start() done(SyncFileItem::SoftError, tr("The file %1 is currently in use").arg(newItem._file), ErrorCategory::GenericError); return; } - } else { - const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(oldFile.toUtf8(), [oldFile, this] (const SyncJournalFileRecord &record) -> void { - const auto oldFileName = record._path; - const auto oldFileNameString = QString::fromUtf8(oldFileName); + } else if (!fileAlreadyMoved) { + qCDebug(lcPropagateLocalRename) << "propagate child items after move from" << existingFile << "to" << targetFile; + const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(previousNameInDb.toUtf8(), [previousNameInDb, this] (const SyncJournalFileRecord &record) -> void { + const auto oldFileNameString = propagator()->adjustRenamedPath(QString::fromUtf8(record._path)); auto newFileNameString = oldFileNameString; - newFileNameString.replace(0, oldFile.length(), _item->_renameTarget); + newFileNameString.replace(0, previousNameInDb.length(), _item->_renameTarget); + + qCDebug(lcPropagateLocalRename) << "child rename from" << oldFileNameString << "to" << newFileNameString; if (oldFileNameString == newFileNameString) { + Q_ASSERT(false); return; } SyncJournalFileRecord oldRecord; - if (!propagator()->_journal->getFileRecord(oldFileName, &oldRecord)) { - qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileName; + if (!propagator()->_journal->getFileRecord(oldFileNameString, &oldRecord)) { + qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileNameString; done(SyncFileItem::NormalError, tr("Could not get file %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } - if (!propagator()->_journal->deleteFileRecord(oldFileName)) { - qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileName; + if (!propagator()->_journal->deleteFileRecord(oldFileNameString)) { + qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileNameString; done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError); return; } diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 4f775c7fc..246d34882 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -1317,6 +1317,61 @@ private slots: QCOMPARE(counter.nMKCOL, 0); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + + void testRenameComplexScenarioNoRecordLeak() + { + FakeFolder fakeFolder{FileInfo{}}; + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.remoteModifier().mkdir("0"); + fakeFolder.remoteModifier().mkdir("0/00 without file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a without file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/aa with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/00 with file"); + fakeFolder.remoteModifier().mkdir("0/00 without file/project/new without file"); + fakeFolder.remoteModifier().insert("0/00 without file/project/00 with file/test.md"); + fakeFolder.remoteModifier().insert("0/00 without file/project/a/a with file/test.md"); + fakeFolder.remoteModifier().insert("0/00 without file/project/a/aa with file/test.md"); + + QVERIFY(fakeFolder.syncOnce()); + + auto itemsCounter = 0; + auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + QCOMPARE(itemsCounter, 12); + + fakeFolder.remoteModifier().rename("0/00 without file/project", "project tests"); + fakeFolder.remoteModifier().rename("project tests/a", "project tests/a empty"); + fakeFolder.remoteModifier().rename("project tests/a empty/a with file", "project tests/a with file"); + fakeFolder.remoteModifier().rename("project tests/a empty/a without file", "project tests/a without file"); + fakeFolder.remoteModifier().rename("project tests/a empty/aa with file", "project tests/aa with file"); + fakeFolder.remoteModifier().rename("project tests/new without file", "project tests/new without file"); + fakeFolder.remoteModifier().rename("0/00 without file", "project tests/00 without file"); + fakeFolder.remoteModifier().rename("0", "project tests/z 0 empty"); + + connect(&fakeFolder.syncEngine(), &OCC::SyncEngine::itemCompleted, this, [&fakeFolder] () { + auto itemsCounter = 0; + auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + if (itemsCounter > 12) { + QVERIFY(itemsCounter <= 12); + } + }); + + QVERIFY(fakeFolder.syncOnce()); + + itemsCounter = 0; + dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; }); + + QVERIFY(dbResult); + QCOMPARE(itemsCounter, 12); + } }; QTEST_GUILESS_MAIN(TestSyncMove)